-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add generic Zino config file #249
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #249 +/- ##
==========================================
- Coverage 98.89% 98.85% -0.03%
==========================================
Files 51 52 +1
Lines 6906 6977 +71
==========================================
+ Hits 6829 6897 +68
- Misses 77 80 +3 ☔ View full report in Codecov by Sentry. |
a0e018c
to
1d4a185
Compare
Quality Gate passedIssues Measures |
If the zino.toml file is used to do the job of |
It can be nice to have an example zino.toml with all the defaults explicitly set and commented out. This is what postgres does. |
from the config file: it could also be beneficial to add options to categories, e.g.
|
i think everything in zino.toml should have it's default fallback in code-defaults. and those should be mandatory to have.
I do not think it should be redundant options in polldevs.cf and zino.toml.. and everythin in polldevs.cf should be directly connected to one or more devices in the polling cycle. |
Also, if multiple config files, all should use the same format unless one of them only contains one string. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're on the right track here, but I believe it should still be possible to run Zino without having a config-file. It should just run off its default values. I might not have been too specific about this in the original issue.
A common strategy for this, used in other projects of ours, is just to define the default config internally (either as a raw representation of the config file contents, or as a default version of the dict structure the file is parsed to), and let an actual config file read operation overwrite those defaults.
(I did put some inline comments, but they must be read in light of this overall comment)
1d4a185
to
b339e77
Compare
For documentation purposes:
|
587687b
to
30a488f
Compare
As a reminder from our meeting last week: When loading the configuration file it should extend the internal default values definded in zino. |
06340ef
to
b730365
Compare
Command line option will still be preferred
3dd1662
to
c167778
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do this :)
Oh well, playing fast and loose this Friday afternoon... @johannaengland left the office and I forgot there was a fixup! commit here :-D |
Closes #224.
A few notes for discussion/information:
eventid_file
is not necessary, since it only keeps track of the last event id, which is something we already cover withlast_event_id
, which is dumped tozino-state.json
New questions that popped up during the fixing/writing tests: